[MRG] Bug on dual potential (constraint violation when some weights are 0) #123
+219
−23
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rflamary
changed the title
ncourty
requested changes
Jan 28, 2020
Collaborator
ncourty
left a comment
ncourty
left a comment
There was a problem hiding this comment.
Nice bug correction @rflamary. Maybe you can be more explicit in the doc why we need this fix (mostly because of the way EMD is coded in C++). In the longer term, should we plan to integrate this patch rather inside the C++ code ?
ot/lp/__init__.py
Outdated
| def estimate_dual_null_weights(alpha0, beta0, a, b, M): | ||
| r"""Estimate feasible values for 0-weighted dual potentials | ||
| The feasible values are computed efficiently bjt rather coarsely. |
ot/lp/__init__.py
Outdated
| \beta_j = \beta_j -v^b_j \quad \text{ if } b_j=0 \text{ and } v^b_j>0 | ||
| In the end the dual potential are centred using function |
ot/lp/__init__.py
Outdated
| :ref:`center_ot_dual`. | ||
| Note that all those updates do not change the objective value of the | ||
| solution but provide dual potential that do not violate the constraints. |
ot/lp/__init__.py
Outdated
| beta0 : (nt,) numpy.ndarray, float64 | ||
| Target dual potential | ||
| a : (ns,) numpy.ndarray, float64 | ||
| Source histogram (uniform weight if empty list) |
Collaborator
There was a problem hiding this comment.
histogram -> distribution ?
weight -> weights
ot/lp/__init__.py
Outdated
| a : (ns,) numpy.ndarray, float64 | ||
| Source histogram (uniform weight if empty list) | ||
| b : (nt,) numpy.ndarray, float64 | ||
| Target histogram (uniform weight if empty list) |
ot/lp/__init__.py
Outdated
| bsel = b != 0 | ||
|
|
||
| # compute dual constraints violation | ||
| Viol = alpha0[:, None] + beta0[None, :] - M |
Collaborator
There was a problem hiding this comment.
can we change the name of the variable here by something more explicit, e.g. constraints_violation ?
ot/lp/__init__.py
Outdated
| # compute dual constraints violation | ||
| Viol = alpha0[:, None] + beta0[None, :] - M | ||
|
|
||
| # Compute worst violation per line and columns |
test/test_ot.py
Outdated
| np.testing.assert_almost_equal(cost1, log['cost']) | ||
| check_duality_gap(a, b, M, G, log['u'], log['v'], log['cost']) | ||
|
|
||
| viol = log['u'][:, None] + log['v'][None, :] - M |
ot/lp/__init__.py
Outdated
|
|
||
| def emd(a, b, M, numItermax=100000, log=False, dense=True): | ||
| def center_ot_dual(alpha0, beta0, a=None, b=None): | ||
| r"""Center dual OT potentials wrt theirs weights |
ot/lp/__init__.py
Outdated
| r"""Center dual OT potentials wrt theirs weights | ||
| The main idea of this function is to find unique dual potentials | ||
| that ensure some kind of centering/fairness. It will help have |
Collaborator
There was a problem hiding this comment.
can you be more explicit on the meaning of 'centering/fairness' ?
Collaborator
Author
|
Thank you @ncourty i took care of your comments. We now have a warning in the emd_c function and the reason why we need to find proper dual potential is discussed. |
rflamary
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I just discovered this important bug in the `ot.emd``function and it feels like opening pandoras's box.
I added in the fisrt commit a test that checks that the constraints are satisfied and we can see in the travis build that the test fails. This comes from the fact that the c++ emd solver is sparse in the sens that if a weight is 0 it will discard the bin and solve a smaller subproblem on the histograms that have non-zero weights. It works very well and is very efficient in practice bug it does not return proper dual variables because it sets the value for the bins with 0 weight to 0 which can lead to constraint violation in practice.
What I will do in this PR:
ot.emdshould work by default (return proper dual variables/subgradients)The post-processing is actually necessary when trying to estimate correct dual variable from the optimal sub-problem if we want to avoid bias in the resulting dual potentials (bnasically everyone at 0 on either the left or right potential).